Skip to content

Conversation

@aceppaluni
Copy link
Contributor

Description:
Add Set_node_account_ids() method to Query and Transaction for retrieving receipts or records.

  • Added methods to both Query and Transaction
  • Added unit test for Query
  • Added unit test for Transaction

Related issue(s):

Fixes #86

Notes for reviewer:
Ran tests for both Query and Transaction

2025-09-08 (2) 2025-09-08

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

aceppaluni and others added 21 commits May 22, 2025 10:26
Signed-off-by: Angelina <aceppaluni@gmail.com>
Signed-off-by: dosi <dosi.kolev@limechain.tech>
Signed-off-by: dosi <dosi.kolev@limechain.tech>
Signed-off-by: dosi <dosi.kolev@limechain.tech>
Signed-off-by: dosi <dosi.kolev@limechain.tech>
Signed-off-by: dosi <dosi.kolev@limechain.tech>
Signed-off-by: dosi <dosi.kolev@limechain.tech>
Signed-off-by: dosi <dosi.kolev@limechain.tech>
Signed-off-by: dosi <dosi.kolev@limechain.tech>
…xamples

Signed-off-by: dosi <dosi.kolev@limechain.tech>
Signed-off-by: dosi <dosi.kolev@limechain.tech>
Signed-off-by: dosi <dosi.kolev@limechain.tech>
@aceppaluni aceppaluni marked this pull request as ready for review September 9, 2025 14:24
Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @accepaluni please remember to pre-fix your PR title eg
feat: add set node account id method
additionally, we now have a changelog, which must be updated with each PR

@nadineloepfe
Copy link
Contributor

Hi @aceppaluni,
are you still working on this?
also, would you mind adding a "feat:" in front of your PR title as well as an entry in the CHANGELOG? thanks

@aceppaluni
Copy link
Contributor Author

Hi @nadineloepfe Yes, my apologies.
I have been a bit busier then normal. Of course, will do, thank you!

@aceppaluni aceppaluni changed the title Add set node account id method feat: Add set node account id method Oct 20, 2025
@exploreriii
Copy link
Contributor

Please push the new changes if you are happy with them and lets run the tests here 👍

@aceppaluni
Copy link
Contributor Author

@exploreriii I pushed the changes and it corrected those first few failing tests. Seems as though another is failing : test_query_retry_on_busy

@aceppaluni
Copy link
Contributor Author

Hi! @nadineloepfe @exploreriii ,
I did a copy/paste of the test_executable.py method and I cannot get the test_query_retry_busy test to pass.

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aceppaluni looking at https://github.com/hiero-ledger/hiero-sdk-python/pull/362/files
it is clear that you've not added the new functionality, please try to copy/paste it again now that a rebase isn't required, it should work easier
There are also errors such as deleted lines from a rebase - sorry!!

@aceppaluni
Copy link
Contributor Author

@exploreriii
Hmm, thats interesting. I wonder if maybe I should have copied more then just query and executable?
And no worries! Rebases are definitely tricky for me.

@exploreriii
Copy link
Contributor

try it again please and see if it works 👍
You can see in /files, query and executable look very different to Nadine's version

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So some of the content in the PR differs from Nadine's pR
https://github.com/hiero-ledger/hiero-sdk-python/pull/853/files#diff-0600439f79ba6386f80cba2dba2dfebdf084869ebbf9943cc3eaf0cad9fd013a
not sure how that happened, but it is different
Some files are the same, meaning maybe only some things were copied over?

self.operator = self.operator or client.operator
self.node_account_ids = list(set(self.node_account_ids))

if not getattr(self, "node_account_ids", None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i notice you have added line 110 to 112 compared to Nadine's version

# and ensures that the correct signatures are used when submitting transactions
self._signature_map: dict[bytes, basic_types_pb2.SignatureMap] = {}
# changed from int: 2_000_000 to Hbar: 0.02
self._default_transaction_fee = Hbar(0.02)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all this section is different to Nadine's version too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def __init__(self) -> None:
    """
    Initializes a new Transaction instance with default values.
    """

    super().__init__()

    self.transaction_id = None
    self.transaction_fee: int | None = None
    self.transaction_valid_duration = 120 
    self.generate_record = False
    self.memo = ""
    self.custom_fee_limits: list[CustomFeeLimit] = []
    # Maps each node's AccountId to its corresponding transaction body bytes
    # This allows us to maintain separate transaction bodies for each node
    # which is necessary in case node is unhealthy and we have to switch it with other node.
    # Each transaction body has the AccountId of the node it's being submitted to.
    # If these do not match `INVALID_NODE_ACCOUNT` error will occur.
    self._transaction_body_bytes: dict[AccountId, bytes] = {}

    # Maps transaction body bytes to their associated signatures
    # This allows us to maintain the signatures for each unique transaction
    # and ensures that the correct signatures are used when submitting transactions
    self._signature_map: dict[bytes, basic_types_pb2.SignatureMap] = {}
    self._default_transaction_fee = 2_000_000
    self.operator_account_id = None
    self.batch_key: Optional[PrivateKey] = None

@aceppaluni
Copy link
Contributor Author

@exploreriii I've copied the files but the workflow is not passing locally.

@exploreriii exploreriii marked this pull request as draft December 7, 2025 21:29
@exploreriii
Copy link
Contributor

ok, would suggest to push so we can see what is going on

aceppaluni and others added 13 commits December 8, 2025 11:21
Signed-off-by: Angelina <aceppaluni@gmail.com>
Signed-off-by: Angelina <aceppaluni@gmail.com>
Signed-off-by: Angelina <aceppaluni@gmail.com>
Signed-off-by: Angelina <aceppaluni@gmail.com>
Signed-off-by: Angelina <aceppaluni@gmail.com>
…election

Signed-off-by: Angelina <aceppaluni@gmail.com>
…ion flow (hiero-ledger#362)

Signed-off-by: Angelina <aceppaluni@gmail.com>
Signed-off-by: Angelina <aceppaluni@gmail.com>
Signed-off-by: Angelina <aceppaluni@gmail.com>
Signed-off-by: Angelina <aceppaluni@gmail.com>
Signed-off-by: Angelina <aceppaluni@gmail.com>
Signed-off-by: Angelina <aceppaluni@gmail.com>
@aceppaluni aceppaluni force-pushed the AddSetNodeAccountIdMethod branch from a0f96ad to c60399f Compare December 8, 2025 16:41
@github-actions
Copy link

github-actions bot commented Dec 8, 2025

Hi, this is WorkflowBot.
Your pull request cannot be merged as it is not passing all our workflow checks.
Please click on each check to review the logs and resolve issues so all checks pass.
To help you:

@aceppaluni
Copy link
Contributor Author

@nadineloepfe We are now down to one failing check. I am not certain as to why as the files were copied the same.

@nadineloepfe
Copy link
Contributor

Hi @aceppaluni :
I hope it's ok that this has been merged now:
#1020

you're commits have been preserved, so you still got the sign off for your work. I therefore close this PR now.
Thank you so much! :)

@aceppaluni
Copy link
Contributor Author

aceppaluni commented Dec 9, 2025

@nadineloepfe Thank you!

Could you tell me what was done to fix the final check? I am trying to take a look but only see changes to the CHANGELOG and had wanted to take it as a learning opportunity :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add set_node_account_ids() functionality to track executed node

4 participants